UI/API: templates ETag + pagination (client cache, 304 handling) #16
Conversation
…iew; show details bottom sheet after import
Summary of ChangesHello @zensgit, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces significant performance and user experience improvements to template management. It implements ETag-based caching and pagination on the backend API for template retrieval, reducing network load and enabling more efficient data handling. Concurrently, the Flutter frontend has been updated to leverage these new API capabilities, providing users with an enhanced template import dialog that supports selection, conflict resolution, and import previews. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces ETag-based caching and pagination for the template listing API, which is a great improvement for performance and client-side efficiency. The changes span both the Rust backend and the Flutter frontend.
My review has identified a few critical issues in the Rust implementation, including a potential panic and a bug that could lead to data inconsistency. There's also significant code duplication that should be addressed. On the Flutter side, the error handling for fetching templates needs to be improved to avoid a confusing user experience. I've also included some suggestions for code simplification.
Please address the critical issues before merging.
| if let Some(classification) = ¶ms.r#type { | ||
| let classification = classification.to_lowercase(); | ||
| if classification != "all" { | ||
| stats_q.push(" AND classification = "); | ||
| stats_q.push_bind(classification); | ||
| } |
There was a problem hiding this comment.
It's correct to lowercase the classification here for a case-insensitive comparison. However, the same logic is missing when applying this filter to the main query builder (around line 139). This inconsistency can cause the total_count to mismatch the number of returned templates, which is a critical bug. Please ensure the filtering logic is identical in both places. This issue also highlights the risk of the duplicated filtering logic.
| let max_updated: chrono::DateTime<chrono::Utc> = stats_row.try_get("max_updated").unwrap_or(chrono::DateTime::<chrono::Utc>::from_timestamp(0, 0).unwrap()); | ||
| let total_count: i64 = stats_row.try_get("total").unwrap_or(0); |
There was a problem hiding this comment.
Using unwrap_or on a Result from try_get will panic if the operation fails (e.g., a column name typo or type mismatch), crashing the request handler. Please use unwrap_or_else to handle the error gracefully. It would also be beneficial to log the error inside the closure for easier debugging.
| let max_updated: chrono::DateTime<chrono::Utc> = stats_row.try_get("max_updated").unwrap_or(chrono::DateTime::<chrono::Utc>::from_timestamp(0, 0).unwrap()); | |
| let total_count: i64 = stats_row.try_get("total").unwrap_or(0); | |
| let max_updated: chrono::DateTime<chrono::Utc> = stats_row.try_get("max_updated").unwrap_or_else(|_| chrono::DateTime::<chrono::Utc>::from_timestamp(0, 0).unwrap()); | |
| let total_count: i64 = stats_row.try_get("total").unwrap_or_else(|_| 0); |
| // Stats for ETag and total (duplicate the same filters) | ||
| let mut stats_q = sqlx::QueryBuilder::new( | ||
| "SELECT COALESCE(MAX(updated_at), to_timestamp(0)) AS max_updated, COUNT(*) AS total FROM system_category_templates WHERE is_active = true" | ||
| ); | ||
| if let Some(classification) = ¶ms.r#type { | ||
| let classification = classification.to_lowercase(); | ||
| if classification != "all" { | ||
| stats_q.push(" AND classification = "); | ||
| stats_q.push_bind(classification); | ||
| } | ||
| } | ||
| if let Some(group) = ¶ms.group { | ||
| stats_q.push(" AND category_group = "); | ||
| stats_q.push_bind(group); | ||
| } | ||
| if let Some(featured) = params.featured { | ||
| stats_q.push(" AND is_featured = "); | ||
| stats_q.push_bind(featured); | ||
| } | ||
| if let Some(since) = ¶ms.since { | ||
| stats_q.push(" AND updated_at > "); | ||
| stats_q.push_bind(since); | ||
| } |
There was a problem hiding this comment.
This block for building the stats_q query duplicates the filtering logic from lines 136-157. This violates the DRY (Don't Repeat Yourself) principle and is error-prone, as demonstrated by the inconsistent classification handling mentioned in another comment. Any future changes to filtering logic will need to be applied in two places, increasing maintenance overhead. Please consider refactoring to a helper function or another pattern to apply filters to both QueryBuilder instances from a single source of truth.
| templates = await CategoryServiceIntegrated().getAllTemplates(forceRefresh: true); | ||
| } catch (_) {} |
There was a problem hiding this comment.
This empty catch block silently swallows any errors that occur while fetching templates. This leads to a poor user experience, as the user will be shown an empty import dialog without any indication of what went wrong. The error should be caught and communicated to the user, for example, by showing a SnackBar, and the loading indicator should be correctly handled.
| templates = await CategoryServiceIntegrated().getAllTemplates(forceRefresh: true); | |
| } catch (_) {} | |
| templates = await CategoryServiceIntegrated().getAllTemplates(forceRefresh: true); | |
| } catch (e) { | |
| if (mounted) { | |
| ScaffoldMessenger.of(context).showSnackBar(SnackBar(content: Text('Error loading templates: $e'))); | |
| } | |
| setState(() { _busy = false; }); | |
| return; | |
| } |
| /// 获取所有系统分类模板 | ||
| Future<List<SystemCategoryTemplate>> getAllTemplates({ | ||
| bool forceRefresh = false, | ||
| }) async { | ||
| return getSystemTemplates(); | ||
| } |
There was a problem hiding this comment.
The getAllTemplates method is a redundant wrapper around getSystemTemplates. It calls getSystemTemplates() with no arguments, which is behavior that getSystemTemplates already supports. This adds an unnecessary layer of indirection. To simplify the service's API, consider removing this method and calling getSystemTemplates() directly where needed.
There was a problem hiding this comment.
Pull Request Overview
Adds ETag-aware, paginated template listing to the API and wires up a minimal UI flow to import templates, with a Flutter service method to compute and handle weak ETags client-side.
- Backend: Add page/per_page/etag query support, compute weak ETag from MAX(updated_at)+COUNT, return 304 on match, and paginate results.
- Flutter service: Add getTemplatesWithEtag and TemplateCatalogResult; compute weak ETag; parse responses.
- UI: Minimal “Import from template library” dialog flow (select, preview via dry-run, import), plus minor refactors.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| jive-flutter/lib/services/api/category_service.dart | Adds ETag+pagination-aware fetch, weak ETag computation, and a result wrapper; moves SystemCategoryTemplate to models. |
| jive-flutter/lib/screens/management/category_management_enhanced.dart | Implements a minimal template import dialog (selection, conflict strategy, preview, import). |
| jive-api/src/handlers/template_handler.rs | Adds page/per_page/etag query params, computes ETag and 304 path, paginates the SQL, and returns last_updated from MAX(updated_at). |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| Future<List<SystemCategoryTemplate>> getAllTemplates({ | ||
| bool forceRefresh = false, | ||
| }) async { |
There was a problem hiding this comment.
The forceRefresh parameter is unused; this method just proxies to getSystemTemplates(). Either remove the parameter or implement refresh behavior (e.g., bypass local cache) to avoid confusion.
| Future<List<SystemCategoryTemplate>> getAllTemplates({ | |
| bool forceRefresh = false, | |
| }) async { | |
| Future<List<SystemCategoryTemplate>> getAllTemplates() async { |
| ); | ||
|
|
||
| if (resp.statusCode == 304) { | ||
| return TemplateCatalogResult(const [], etag, true, 0, page, perPage); |
There was a problem hiding this comment.
On 304, returning items as [] and total as 0 can mislead consumers that don't check notModified. Consider making total nullable (int?) or adding a dedicated factory/flag behavior so callers can reliably differentiate 'unknown/unchanged' from 'empty/zero'. At minimum, document that items and total should be ignored when notModified is true.
| return TemplateCatalogResult(const [], etag, true, 0, page, perPage); | |
| return TemplateCatalogResult(const [], etag, true, null, page, perPage); |
| @@ -1,60 +1,184 @@ | |||
| import 'package:flutter/material.dart'; | |||
| import 'package:flutter_riverpod/flutter_riverpod.dart'; | |||
| import '../../models/category.dart'; | |||
There was a problem hiding this comment.
This import appears unused in this file. Remove it to keep imports clean.
| import '../../models/category.dart'; |
| 'per_page': perPage, | ||
| if (group != null) 'group': group, | ||
| if (classification != null) | ||
| 'type': classification.toString().split('.').last, |
There was a problem hiding this comment.
Prefer using the enum's name getter instead of string-splitting: 'type': classification.name. It's more robust and readable.
| 'type': classification.toString().split('.').last, | |
| 'type': classification.name, |
| let response = TemplateResponse { | ||
| templates, | ||
| version: "1.0.0".to_string(), | ||
| last_updated: chrono::Utc::now().to_rfc3339(), | ||
| total: total.0, | ||
| last_updated: max_updated.to_rfc3339(), | ||
| total: total_count, |
There was a problem hiding this comment.
Consider surfacing the computed ETag in successful responses (e.g., include an etag field in TemplateResponse or set the ETag header) to help clients avoid re-computing it. Returning the header is preferable for intermediaries and caches.
| // Stats for ETag and total (duplicate the same filters) | ||
| let mut stats_q = sqlx::QueryBuilder::new( | ||
| "SELECT COALESCE(MAX(updated_at), to_timestamp(0)) AS max_updated, COUNT(*) AS total FROM system_category_templates WHERE is_active = true" | ||
| ); | ||
| if let Some(classification) = ¶ms.r#type { | ||
| let classification = classification.to_lowercase(); | ||
| if classification != "all" { | ||
| stats_q.push(" AND classification = "); | ||
| stats_q.push_bind(classification); | ||
| } | ||
| } | ||
| if let Some(group) = ¶ms.group { | ||
| stats_q.push(" AND category_group = "); | ||
| stats_q.push_bind(group); | ||
| } | ||
| if let Some(featured) = params.featured { | ||
| stats_q.push(" AND is_featured = "); | ||
| stats_q.push_bind(featured); | ||
| } | ||
| if let Some(since) = ¶ms.since { | ||
| stats_q.push(" AND updated_at > "); | ||
| stats_q.push_bind(since); | ||
| } |
There was a problem hiding this comment.
The filters are duplicated for stats and data queries, causing two scans. Consider a single CTE or subquery to compute rows once, e.g., WITH filtered AS (...) SELECT COUNT(*), MAX(updated_at) FROM filtered; and SELECT ... FROM filtered ORDER BY ... LIMIT/OFFSET. This reduces DB work and avoids filter-drift bugs.
| /// 模板目录结果（含 ETag） | ||
| class TemplateCatalogResult { | ||
| final List<SystemCategoryTemplate> items; | ||
| final String? etag; | ||
| final bool notModified; | ||
| final int total; |
There was a problem hiding this comment.
Please document the semantics when notModified is true (e.g., items/total should be ignored and callers should use their cache). Given 304 returns use total=0, explicit docs help prevent misuse.
| /// 模板目录结果（含 ETag） | |
| class TemplateCatalogResult { | |
| final List<SystemCategoryTemplate> items; | |
| final String? etag; | |
| final bool notModified; | |
| final int total; | |
| /// 模板目录结果（含 ETag） | |
| /// | |
| /// When [notModified] is true, this indicates that the data has not changed since the last request (e.g., HTTP 304). | |
| /// In this case, [items] and [total] should be ignored, and callers should use their cached data instead. | |
| /// Only [etag], [notModified], [page], [perPage], and [error] are meaningful when [notModified] is true. | |
| class TemplateCatalogResult { | |
| /// The list of category templates. | |
| /// | |
| /// When [notModified] is true, this value should be ignored and callers should use their cached items. | |
| final List<SystemCategoryTemplate> items; | |
| /// The ETag value for cache validation. | |
| final String? etag; | |
| /// Whether the data was not modified (e.g., HTTP 304). | |
| /// | |
| /// If true, [items] and [total] should be ignored and callers should use their cached data. | |
| final bool notModified; | |
| /// The total number of items. | |
| /// | |
| /// When [notModified] is true, this value should be ignored and callers should use their cached total. | |
| final int total; |
1 participant
Adds getTemplatesWithEtag(page/per_page/etag) to CategoryService. Computes weak ETag from last_updated+total, handles 304 by signaling notModified for caller-side cache.\nNext: wire into template dialogs to use ETag and paginated loading.